ci: move concurrency dedup from trigger to review workflow#2789
Conversation
defer PR comment cancellation to the expensive review job instead of the cheap trigger run, eliminating cosmetic CI failures when multiple PR events land close together
| types: [completed] | ||
|
|
||
| concurrency: | ||
| group: pr-review-${{ github.event.issue.number || github.event.workflow_run.pull_requests[0].number || github.run_id }} |
There was a problem hiding this comment.
[MEDIUM] Fork PR workflow_run events fall back to run_id, defeating deduplication
For fork PRs — which are the primary use-case for the trigger-based workflow_run path — GitHub deliberately leaves workflow_run.pull_requests as an empty array for security reasons. As a result, github.event.workflow_run.pull_requests[0].number evaluates to empty in a GitHub Actions expression, and || falls through to github.run_id.
Because run_id is unique per run, every fork-PR review run lands in its own concurrency group, making cancel-in-progress: true completely ineffective for fork PRs. If opened + review_requested fire back-to-back for a fork PR (the exact scenario this PR aims to fix), both pr-review.yml runs will proceed independently instead of the first being cancelled.
Suggested fix: extract the PR number earlier in the workflow — e.g., store it in the context artifact during save-context and pass it as a workflow input — or use a jq expression on the artifact file to derive the group key.
Alternatively, a workaround for the workflow_run path is to parse the PR number out of the triggering workflow's inputs/artifacts rather than relying on workflow_run.pull_requests.
| types: [completed] | ||
|
|
||
| concurrency: | ||
| group: pr-review-${{ github.event.issue.number || github.event.workflow_run.pull_requests[0].number || github.run_id }} |
There was a problem hiding this comment.
[LOW] Issue numbers and PR numbers share the same concurrency group key namespace
GitHub issues and PRs share a single number sequence per repository. An issue_comment event on a plain issue #42 will produce group key pr-review-42, which is identical to the group key a workflow_run triggered by PR #42 would produce.
The workflow-level concurrency block is evaluated before any job-level if: conditions, so a workflow_run for PR #42 could cancel a concurrently-running issue_comment-triggered workflow for issue #42 (or vice versa) even when they are unrelated. In practice the review job's if: condition prevents any work from happening for non-PR issue comments, but the cancellation of the competing workflow run is still visible in the checks list.
Suggestion: Distinguish event sources in the group key, e.g.:
group: pr-review-${{ github.event_name }}-${{ github.event.issue.number || github.event.workflow_run.pull_requests[0].number || github.run_id }}This limits any cancellation to runs of the same event type, though the fork-PR limitation still applies to workflow_run events.
The pr-review-trigger workflow intentionally runs all events to completion (it's cheap, just saves context). Deduplication happens downstream in pr-review.yml where the expensive AI review runs. This updates the linter to skip the concurrency check for pr-review-trigger.yml, resolving the failing lint check. See PR docker#2789 for the rationale behind moving concurrency control from the trigger to the review workflow.
3 participants
What
Move the GitHub Actions
concurrencyblock frompr-review-trigger.yml(the cheap "save-context" job) topr-review.yml(the expensive AI review job).Why
Currently, when multiple PR events land on the same PR in quick succession — typically
opened+review_requestedwhen reviewers are pre-assigned at PR creation, or severalpull_request_review_commentevents from inline review comments — the olderPR Review - Triggerruns get cancelled bycancel-in-progress: true.GitHub renders cancelled check runs with the same red ❌ as failed runs, so PRs end up with a
save-contextcheck that looks like it failed but actually just got superseded. See for example the failingsave-contextcheck on #2785, where two trigger runs fired (openedthenreview_requested) and the second was cancelled with:Nothing was actually broken — the successful run produced the artifact, and
pr-review.ymlran correctly downstream — but the noise on the PR checks list is confusing.How
The trigger workflow only saves a small context artifact, so it's cheap to let every event run to completion. The deduplication that actually matters is on the review job (which is what costs API calls / compute), so the concurrency block belongs there.
The new group key in
pr-review.ymlcovers all three triggering event sources:issue_comment→github.event.issue.numberworkflow_run(from the trigger) →github.event.workflow_run.pull_requests[0].numbergithub.run_id(so unrelated runs never share a group)Net effect
opened+review_requestedback-to-back → bothsave-contextruns succeed (no more red ❌), and only the latest review actually runs. ✅